Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add TestServer support for GraphQLHttpClient #354 #357

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add TestServer support for GraphQLHttpClient #354 #357

wants to merge 3 commits into from

Conversation

andreyleskov
Copy link

@andreyleskov andreyleskov commented Jun 29, 2021 •
edited
Loading

This PR makes GraphQLHttpClient usable with TestServer, targeting #354

Usage

  TestServer testServer; //needs initialization
  GraphQLHttpClientOptions options; //needs initialization
  IGraphQLWebsocketJsonSerializer serializer;  //needs initialization

  var graphQLHttpClient = testServer.CreateGraphQLHttpClient(options,serializer);

Implementation
GraphQLHttpClient and GraphQLHttpWebSocket have a new internal constructor with a new factory parameter for connected WebSocket creation.

Func<Uri, CancellationToken,Task<WebSocket>> connectedWebSocketFactory

This parameter is used to provide TestWebSocket instance from TestServer.
There is a new project GraphQL.Client.TestHost with the extensions method to configure this factory for a given TestServer.

Discussion

  1. Need to check if net461 build is ok, as I checked it only with Mono under MacOS, would be nice to check with full .Net.
  2. There is CanHandleConnectionTimeout integration test that I could not make work for TestServer. Seems like TestServer does not drop WebSocket connection on dispose. I could be wrong. Any help or suggestions are welcome. As a shortcut, I moved this test out of scope for TestServer
  3. Way to inject WebSocket into GraphQLHttpClient. Now it is dictated by TestServer's WebSocketClient, and, maybe, there is a better way other than GraphQLHttpClient internal constructor requiring complex factory setup and separate project usage.
  4. Tests stability, as mentioned in Unstable Integration Tests #161. I can see the same behavior for TestServer and think it is caused by synchronization inside tests themselves working with the same server instance. This issue is not targeted by the PR, but it makes this hypothesis check easier a bit by allowing to run a dedicated TestServer for each test.

@andreyleskov andreyleskov marked this pull request as ready for review June 29, 2021 10:29
@andreyleskov andreyleskov mentioned this pull request Jun 29, 2021
Open
rose-a
rose-a reviewed Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@rose-a rose-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, just wanted to let you know that im currently looking into your MR...

The results from your new tests and those from the legacy ones are the same (which is good), but they are also noteably faster. I suspect the errors mentioned in #161 are the result of a threading issue with the test classes for the IObservables, since the trigger for the observable happens in the same thread in which the expected result is awaited, which is a totally untypical use of System.Reactive. They're also sporadic (it's not always the same test that fails), which makes it even harder to find them.
I'm having the same issues in other projects im working on where I'm trying to test observables in a similar way.

I think the injection of a websocket factory might actually be a nice improvement, since it would allow us to move the dependency on System.Net.WebSockets.Client.Managed and all that conditional #if NETFRAMEWORK stuff to a separate package which is only ever needed when targeting Windows 7.

tests/GraphQL.Integration.Tests/Helpers/TestServerTestFixture.cs Outdated Show resolved Hide resolved
@rose-a rose-a changed the base branch from master to release-v4 September 14, 2021 08:38
@rose-a
Copy link
Collaborator

rose-a commented Sep 14, 2021 •
edited
Loading

@andreyleskov please rebase your branch on release-v4

already done

rose-a and others added 2 commits September 14, 2021 11:43
@rose-a
use web socket factory
2f4f7d6
@andreyleskov @rose-a
Update tests/GraphQL.Integration.Tests/Helpers/TestServerTestFixture.cs
bdf1114 
Good improvement, I missed hard-coded localhost.

Co-authored-by: Alexander Rose <[email protected]>
sungam3r
sungam3r reviewed Oct 29, 2021
View reviewed changes
src/GraphQL.Client/Websocket/GraphQLHttpWebSocket.cs
@@ -442,8 +387,7 @@ private async Task ConnectAsync(CancellationToken token)
{
await BackOff();
_stateSubject.OnNext(GraphQLWebsocketConnectionState.Connecting);
Debug.WriteLine($"opening websocket {_clientWebSocket.GetHashCode()} (thread {Thread.CurrentThread.ManagedThreadId})");
await _clientWebSocket.ConnectAsync(_webSocketUri, token);
_clientWebSocket = await _webSocketFactory.ConnectAsync(_webSocketUri, token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rose-a @andreyleskov regarding 4. Tests stability:

I recommend you ALWAYS use ConfigureAwait(false) in library code especially when using XUnit package as a test framework because of Xunit.Sdk.MaxConcurrencySyncContext. In my case maximumConcurrencyLevel was 6 and some tests randomly failed. So please look through entire codebase and add ConfigureAwait(false) to awaitable calls. Perhaps it will help.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the comment, it makes sense from my point of view. This PR is following the existing await style and focuses on just WebSockets. I assume repository owners prefer to do repository-wide changes as await style in a dedicated PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im on to this in in #370...

sungam3r
sungam3r reviewed Oct 29, 2021
View reviewed changes
tests/GraphQL.Integration.Tests/WebsocketTests/BaseWithTimeout.cs

namespace GraphQL.Integration.Tests.WebsocketTests
{
public abstract class BaseWithTimeout:Base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public abstract class BaseWithTimeout:Base
public abstract class BaseWithTimeout : Base

@rose-a rose-a changed the base branch from release-v4 to master October 31, 2021 07:58
@rose-a rose-a mentioned this pull request Oct 31, 2021
Open
@andreyleskov
Copy link
Author

Hey guys, can I do smth to make this PR move? Please let me know

@jerry2007
Copy link

jerry2007 commented Mar 15, 2023 •
edited
Loading

Hello, what about this PR. I would like to use this...
@rose-a ????

@alexander-jesner-AP
Copy link

Hi, @rose-a, is it possible to merge this PR? If not, how can we help to push it towards being mergeable?

@tomachristian
Copy link

This would have been a lifesaver...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sungam3r sungam3r sungam3r left review comments

@rose-a rose-a rose-a left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@andreyleskov @rose-a @jerry2007 @alexander-jesner-AP @tomachristian @sungam3r